Review: PR #67 — Add SWOPP3 analysis script and violations module

Scope: 2 new files, +2974 lines, 1 commit, targeting feat/swopp3-clean

What's good

• routetools/violations.py is well-structured: typed dataclasses, Protocol for land-checker, proper separation of concerns, imports from routetools.weather constants (not hardcoded thresholds).
• scripts/swopp3_analysis.py is thorough — 13 publication-quality figures with consistent IE branding, good docstrings, graceful degradation for missing data.
• Clean merge against current feat/swopp3-clean.
• Good module docstring in violations.py with usage documentation.
• Threshold constants reused from routetools.weather.DEFAULT_TWS_LIMIT / DEFAULT_HS_LIMIT — consistent with the CodaBench scorer.

Issues to address

1. No tests for violations.py (blocking)

violations.py is being added to the core library (routetools/), not scripts. Per repo conventions, library modules need test coverage. There should be at least:

• count_summary_weather_violations — unit test with a temp CSV
• count_land_violations — unit test with mock land checker
• count_folder_violations — integration test with a small temp folder structure
• find_team_prefix — test with known file patterns + FileNotFoundError case
• is_gc_case — quick parametric test
• ScenarioViolationCounts.total_violations / total_penalty — property tests
• format_grouped_violation_table / write_grouped_violation_csv — roundtrip test

2. Very stale base — needs rebase (blocking)

The branch diverges from a point before all test files were added — the diff shows 5213 deleted test lines across 18 files. This means the branch is forked from before:

• USNYS port fix
• MWA wrapping fix
• FMS branch merge (PR #64)
• Circular interpolation tests
• All current test infrastructure

Please rebase onto current feat/swopp3-clean so the branch includes these and tests pass in isolation.

3. Global mutable state in swopp3_analysis.py

OUTPUT_DIR: Path = _REPO_ROOT / "output"
FIGS_DIR: Path = _REPO_ROOT / "output" / "analysis"
# ...
def main():
    global OUTPUT_DIR, FIGS_DIR

Every figure function reads OUTPUT_DIR / FIGS_DIR as globals. This makes the functions untestable in isolation and fragile. Consider passing these as parameters (or a simple config dataclass) through main() → figure functions.

4. swopp3_analysis.py is 2353 lines — consider splitting

The script handles 13 figures + summary table + data loading + style. Consider:

• Extract shared data loading into a small helper module (or reuse parts of violations.py)
• Group related figures into smaller modules

Not blocking, but the current size will make maintenance painful.

5. Hardcoded experiment folder names

EXPERIMENTS = {
    "no_penalty": {"folder": "swopp3_no_penalty", ...},
    "no_penalty_fms": {"folder": "swopp3_no_penalty_fms", ...},
    ...
}

The 4 experiment folder names and the team prefix IEUniversity-1 are hardcoded in both files. If PR #65 (experiment config profiles) lands first, consider reading folder names from config.toml instead of duplicating them.

6. violations.py duplicates ERA5 loading pattern

_loadable_era5_paths() and load_default_weather_resources() duplicate logic already in scripts/swopp3_run.py and routetools/era5/loader.py. Consider refactoring the shared ERA5 path resolution into a single utility.

7. load_tracks() uses random_state=42 without documenting why

sample = summary.sample(min(n_sample, len(summary)), replace=False, random_state=42)

This is fine for reproducibility but the magic number should have a comment noting it's for deterministic track sampling in figures.

8. Cartopy + shapefile as implicit dependencies

violations.py imports cartopy and shapefile/shapely at function level, which is good for lazy loading. But neither appears in pyproject.toml dependencies. They should be listed as optional extras or documented as required for the violations module.

Minor nits

• _red_line_rule() is defined but never called — dead code.
• _outlier_cap name suggests it returns capped data, but it returns a boolean mask. Consider _outlier_mask.
• The PR description says "Violation counts validated against expected totals from output/cmaes_weather" — this validation should be captured as an automated test, not a one-time manual check.

Suggested actions

1. Rebase onto current feat/swopp3-clean (blocking)
2. Add tests/test_violations.py with unit tests for the core library module (blocking)
3. Pass output paths as parameters instead of globals
4. Consider extracting shared ERA5 loading to avoid duplication

Review of #67: Add SWOPP3 analysis script and violations module

Good work overall — the violations.py module is well-structured with clean separation between threshold counting, land checking, and smooth penalty accumulation. The test coverage for violations.py is solid.

Summary

Approve with minor suggestions. Nothing is a blocker; the code is functional and correct.

What works well

• violations.py has a clean public API (count_folder_violations, format_grouped_violation_table, write_grouped_violation_csv)
• The Codabench subsampling rule (step = max(1, len(waypoints) // 50)) is faithfully reproduced
• ScenarioViolationCounts as a frozen dataclass is the right pattern
• loadable_era5_paths() in era5/loader.py is a useful generalized helper
• test_violations.py covers the key paths well (235 lines of solid tests)
• The analysis script produces publication-quality figures with consistent IE branding

Issues (all minor)

1. Unused case_id parameter in count_summary_weather_violations() (see inline comment)
2. travel_time unit ambiguity: The code correctly passes passage_hours, matching how the rest of the codebase works — but wind_penalty_smooth's docstring says "seconds". Worth a clarifying comment (see inline)
3. pd.concat in a loop in fig_penalty_tradeoff() — O(n²) pattern, easy to replace with list + single concat
4. Global warnings.filterwarnings("ignore") — could mask real warnings when importing individual figure functions; better scoped inside main()
5. No test for loadable_era5_paths() — new public function in era5/loader.py with branching logic (regex match, next-year file, glob fallback) that would benefit from a few unit tests
6. importlib test pattern — fragile for testing script internals; consider extracting shared helpers into an importable module

Questions

• The analysis script hardcodes 4 experiment configs (no_penalty, no_penalty_fms, penalty, penalty_fms). With the penalty sweep work we're doing now, we'll likely want more configs. Is the plan to parametrize these later via config.toml, or is this script specifically for the publication comparison?
• load_default_land_checker() creates a unary_union of all 10m Natural Earth shapes on every call (no caching). For a one-shot script this is fine but if count_folder_violations is called in a loop over many folders, the caller should pre-build the checker. Worth noting in the docstring?